FIX: Update implementation of ndim property of transforms
#197
Conversation
Tranformbase -> now raises TypeError; Affine -> overshadows TransformBase's property and returns self._matrix.ndim + 1. To be applied to LinearTransformMapping (Currently TypeError)
Added two tests for ndim: assert nitl.Affine().ndim == 3; assert nitl.LinearTransformsMapping([nitl.Affine()]).ndim == 4 .
jmarabotto
force-pushed
the
Implement_ndim
branch
from
1cdd8fe to
3d40b93
Compare
| """Access the internal representation of this affine.""" | ||
| return self._matrix.ndim + 1 |
There was a problem hiding this comment.
Surely -1? A 4x4 affine represents a transform in 3D space, and nothing intelligible I'm aware of in 5 dimensions.
Also this docstring is off.
There was a problem hiding this comment.
Oh I see, it's a 2D matrix, so this works out to 3, but does so in all cases. What you actually want is self._matrix.shape[0] - 1
There was a problem hiding this comment.
This will not work for 2D transforms. But it does work for >2D:
- self._matrix is 4x4 in 3D transforms (+1)
- self._matrix is tx4x4 in 4D transforms (3D + t)
- self._matrix is tx4x4x2 in 5D transforms (3D + t + complex numbers).
5D could become "a thing" when we want to realign 3D+t images reconstructed with magnitude and phase (or complex). This looks more plausible than generalization to 2D images (2D, 2D+t, 2D+t+part).
There was a problem hiding this comment.
Okay, so we're not considering e.g., a 5x5 affine matrix for transforms of 4D spaces, but a collection of 4x4 affines for a collection of 3D volumes.
If you want to be really general, you could treat it as self._matrix.shape[0] + self._matrix.ndim - 3, which would handle collections of any number of ND affine spaces. But I'm okay keeping this simplification and assuming 3D volumes for now.
As an aside: I don't really understand why you would treat the real and imaginary parts of complex numbers as being an extra dimension in this context. The interpolation would need to account for the complex plane (though on first thought, independent interpolation of real and imaginary components should work...) , but I don't see why the transform would.
There was a problem hiding this comment.
Okay, so we're not considering e.g., a 5x5 affine matrix for transforms of 4D spaces, but a collection of 4x4 affines for a collection of 3D volumes.
Correct
I don't really understand why you would treat the real and imaginary parts of complex numbers as being an extra dimension in this context.
The fact that this ndim could be 5D does not mean that the transform should use all the dimensions. I agree you (in principle) don't want to interpolate in between imaginary and real part.
If you want to be really general, you could treat it as
self._matrix.shape[0] + self._matrix.ndim - 3, which would handle collections of any number of ND affine spaces. But I'm okay keeping this simplification and assuming 3D volumes for now.
I think your implementation is better; why keep the simplification once you've written a better alternative down here?
Resolves: #196 .